Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

stop effectively requiring engine_exchangeTransitionConfigurationV1 #320

Closed
wants to merge 2 commits into from
Closed

stop effectively requiring engine_exchangeTransitionConfigurationV1 #320

wants to merge 2 commits into from

Conversation

tersec
Copy link
Contributor

@tersec tersec commented Nov 3, 2022

engine_exchangeTransitionConfigurationV1 is not necessary anymore, so compatibly begin de-emphasizing it. This approach allows a staging whereby it becomes more optional for both CLs and ELs, but that both CLs and ELs should converge to neither sending nor requiring engine_exchangeTransitionConfigurationV1.

A future PR, once this has progressed, can remove the requirement for ELs to process this message in any particular, way, but that currently creates compatibility challenges within the existing ecosystem.

ajsutton
ajsutton reviewed Nov 3, 2022
View reviewed changes
Copy link

@ajsutton ajsutton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strongly in favour of moving towards removing this call and the first step has to be for ELs to stop complaining if it's not called.

src/engine/specification.md Outdated

5. Execution Layer client software **SHOULD** surface an error to the user if it does not recieve a request on this endpoint at least once every 120 seconds.
5. Execution Layer client software **SHOULD NOT** surface an error to the user if it does not recieve a request on this endpoint at least once every 120 seconds.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would make this MUST NOT so that CLs can stop calling it and know the EL won't complain (given enough time for updates to happen etc).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree: a640d23

@mkalinin
Copy link
Collaborator

I am slightly against changing the spec of the method. I would rather wait for #321 to take into effect as it tends to bring deprecation procedure into Engine API development process.

IMO, we should not change the spec in order to deprecate methods. In this particular case we may do the procedure in the following steps:

  1. Agree that the method is deprecated and upgrade CL clients accordingly, so they won't complain if method doesn't exist but will be still calling it if it does
  2. Remove method from EL clients

@mkalinin mkalinin mentioned this pull request Dec 21, 2022
Closed
@tersec
Copy link
Contributor Author

tersec commented Jan 31, 2023

I am slightly against changing the spec of the method. I would rather wait for #321 to take into effect as it tends to bring deprecation procedure into Engine API development process.

IMO, we should not change the spec in order to deprecate methods. In this particular case we may do the procedure in the following steps:

1. Agree that the method is deprecated and upgrade CL clients accordingly, so they won't complain if method doesn't exist but will be still calling it if it does

2. Remove method from EL clients

Works for me.

@mkalinin mkalinin mentioned this pull request Feb 3, 2023
Closed
@mkalinin
Copy link
Collaborator

Closing in favour of deprecation notices in #420

@mkalinin mkalinin closed this Jun 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ajsutton ajsutton ajsutton left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tersec @mkalinin @ajsutton